Skip to content

Remove dead code from bun_bundler, the JS parser, the help command, and PackageJSONEditor - #44687

Open
robobun wants to merge 1 commit into
mainfrom
robobun/fd900f23/dead-code-sweep
Open

robobun wants to merge 1 commit into
mainfrom
robobun/fd900f23/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Behaviour change: none

Problem

  • Five groups of code in 14 files cannot run, or compute a value that nothing reads.
  • No lint reports them. Each has a textual use that rustc accepts: a comparison with a variant that nothing constructs, a flag with one value, a Default impl that nothing calls.

Fix

  • Delete the five groups: 164 lines removed, 23 added. The Notes give each group with its proof.
  • Each proof is local: a variant with no constructor, a field with one value, an empty block, an impl with no caller.
  • Verified: bun bd, bun run rust:check-all (12 targets), cargo clippy, the tests in Notes, and identical bundler, transpiler and help output against the base commit's canary.
  • Self-reviewed: 4 concerns raised, 3 addressed. The fourth (a lint for this class of code) is a separate task.

Background

  • The workspace denies dead_code. But rustc counts e == Error::EmptyAST as a use of the variant, and a store as a use of a field.
  • esbuild warns about a return that a line break separates from its expression. Bun's parser carries the option that turns such warnings off, and no caller turns them on.
  • A comparison with all open pull requests took out each candidate that another one deletes or rewrites (Notes).

Downsides

  • None found. Checked each removed name with rg over src/, packages/ and build/debug/codegen/, and each removed branch for a second path that reaches it.
Notes

Removed, one group each

  1. parse_task::ResultValue::Empty, bun_bundler::Error::EmptyAST and bun_bundler::Error::ModuleNotFound (33 lines). EmptyAST has no constructor: the only expression that names it is if e == crate::Error::EmptyAST in ParseTask.rs, and no From impl or name table produces it. That if held the one constructor of ResultValue::Empty. So the variant, its three match arms (source_index, drop_result_owned_fields, on_parse_task_complete) and the error variant go. ModuleNotFound of the same enum has no constructor either. The bundler reports a missing module through Error::Resolver(bun_resolver::Error::ModuleNotFound).
  2. esbuild's "weird code" warnings in the JS parser (53 lines). Options.suppress_warnings_about_weird_code is true in Options::default(), Options::init, the literal in src/bundler/transpiler.rs, and the two assignments in function_identities.rs and pm_diff_normalize.rs. clone_for_lazy_export copies it. Its two readers are the ASI warning after a bare return (parse/mod.rs) and an !a instanceof b block whose body is a TODO comment (parse_suffix.rs). With them go P::latest_return_had_semicolon (its one reader was the ASI block), the local return_without_semicolon_start, and the option. The !a in b block of sfx_t_in is the same empty TODO block without the option check, so it goes too.
  3. help_command::Reason::InvalidCommand (31 lines net, src/runtime/cli/mod.rs). Nothing constructs it. exec() and the two other callers pass Reason::Explicit. The enum, the reason parameter, the Uh-oh not sure what to do with that command arm and the exit(1) branch go. print_cli_helptext! loses its printer parameter, because pretty! is its one printer now.
  4. impl Default for linker_graph::File (14 lines). Nothing calls it. LinkerGraph::load creates the rows zeroed, and MultiArrayList<File> has no File: Default bound.
  5. keep_catalog_reference in PackageJSONEditor::edit (10 lines). The flag can be true only when manager.subcommand == Subcommand::Update. Its one reader is the last operand of a condition that has manager.subcommand != Subcommand::Update before it.

Open pull requests

Verified and left out, because an open pull request deletes or changes the same lines

Leads, not proved to the end or not removable without a conflict

  • Options.keep_names and Options.transform_only of the parser (parse_entry.rs) are written and never read. The removal builds on 12 targets, but it conflicts with install: fix regressions and older bugs found after the package manager consolidation #45307 and js_parser: validate TypeScript class member modifiers like esbuild #40817, which both merge cleanly today, so it is not in this PR.
  • LinkerOptions.mode is always LinkerOptionsMode::Bundle, so should_strip_exports in convertStmtsForChunk.rs is always true. The removal dedents about 100 lines in a file that 18 open pull requests touch.
  • ServerComponentParseTask::Data::ClientEntryWrapper is constructed only as the placeholder of impl Default. The removal needs a constructor in place of that impl.
  • NetworkTask.apply_patch_task is written and never read. PatchTask::new_apply_patch_hash, which builds the value, calls compute_cache_dir_and_subpath, so its side effects need a review first.
  • The "merge adjacent @layer rules" block of findImportedFilesInCSSOrder.rs never runs, because wip_order is always empty there. esbuild's version of the block appends each entry to wip_order. This is a missing step of the port, not dead code.

Self-review

Four concerns survived the review, and the seven proofs of the first version held when they were derived again.

  • The description listed three new conflicts, and a merge would have caused more. Addressed: the branch is rebased on main after bun check: fix more differences from tsc #44685 merged, two groups and two fields that collide with pull requests that merge cleanly are out, and the list above comes from trial merges with every open pull request.
  • Dead code of the same kind stayed next to the hunks. Addressed: the !a in b block and Error::ModuleNotFound are in. Options.keep_names and Options.transform_only are named under Leads with the reason.
  • The branch had to wait for bun check: fix more differences from tsc #44685, which moved a line that this PR deletes. Addressed by the rebase. That line's file no longer exists on main.
  • A lint could find this class of code (a flag with one value, a variant without a constructor) and stop new cases. Not addressed here: it is a change to the mordant lint pack, not to this repository's sources.

How the candidates were found

  • Readers went through 24 chunks of Rust (about 180,000 lines in src/install, src/runtime/cli, src/runtime/api, src/bundler and part of src/runtime/node) and listed about 140 places where a value has one possible state.
  • A rustc driver recorded each field read and write, each variant construction and the constant arguments of each call in the workspace. It found the write-only fields and the variants without a constructor. Its output did not survive a reset of the work container, so each group above was proved again by hand on current main.
  • Not covered in this run: src/runtime/webcore, src/runtime/server, the rest of src/js_parser, src/http, src/runtime/shell, src/sql, src/css, the C++ bindings and src/js.

Verification

The base is main at 5f5799e.

  • bun bd passes (debug build with ASAN). bun run rust:check-all passes for 12 of 12 targets. cargo check --workspace passes with --all-targets, with --cfg bun_asan --cfg bun_debug --cfg socket_fault_injection, and with --release. cargo clippy --workspace and cargo fmt --all --check are clean.
  • Tests with the debug build, 0 fail: test/bundler/bundler_edgecase.test.ts (182 pass), test/bundler/transpiler/transpiler.test.js (238 pass), test/cli/bun.test.ts (47 pass), test/cli/install/bun-update.test.ts (173 pass), test/cli/install/catalogs.test.ts (146 pass).
  • Probes against the canary binary of the base commit (1.4.4-canary.1+5f5799ed1):
    • bun build of a module graph with an empty .js file, a comment-only file, an empty .ts file and an empty .json file, in eight modes (default, --no-bundle, --minify, --format=cjs, --target=node with inline and with linked source maps, --splitting, --target=bun --minify-syntax): byte-identical output, no warning in either.
    • Bun.Transpiler.transformSync of five inputs with a return before a line break, !a instanceof b and !a in b: identical output.
    • bun --help: the same lines, apart from the version, the random example package names, and one flag that only debug builds have.
  • The first version of this branch (before the rebase) passed CI in build 123735.

@robobun

robobun commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. CI is green on 6ad24ed (build 125952: 181 of 181 jobs passed).

How each removal was checked: every removed name was searched in src/, packages/ and build/debug/codegen/, and each removed branch was traced to its only writers and callers (the proofs are in the Notes of the description). The branch builds with bun bd, passes bun run rust:check-all on 12 targets, and the test files listed in the description pass with the debug build. The debug build and the canary binary of the base commit give identical bundler, transpiler and help output.

Five tests failed once in that build and passed on retry: disallow-code-generation-from-strings.test.ts, module-graph.test.ts, bake/deinitialization.test.ts, worker_threads.test.ts and test-fs-read-stream-pos.js. None of them runs code that this PR changes.

@robobun

robobun commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:37 AM PT - Oct 15th, 2026

✅ @robobun, your commit 6ad24edca44ba6425461c52d986cfa05b22c4fa8 passed in Build #125952! 🎉


🧪   To try this PR locally:

bunx bun-pr 44687

That installs a local version of the PR into your bun-44687 executable, so you can run:

bun-44687 --bun

@robobun
robobun force-pushed the robobun/fd900f23/dead-code-sweep branch from 5b23e7a to 6ad24ed Compare October 15, 2026 13:59
@robobun robobun changed the title Remove dead code from the bundler, the JS parser, Glob.scan, bun --help, and bun_install Remove dead code from bun_bundler, the JS parser, the help command, and PackageJSONEditor Oct 15, 2026
@robobun
robobun marked this pull request as ready for review October 15, 2026 14:03
@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 5b21af06-2636-443f-83d2-0a6492b9a3bb
📥 Commits

Reviewing files that changed from the base of the PR and between a407eae and 6ad24ed.

📒 Files selected for processing: 14
  • src/bundler/LinkerGraph.rs
  • src/bundler/ParseTask.rs
  • src/bundler/bundle_v2.rs
  • src/bundler/error.rs
  • src/bundler/transpiler.rs
  • src/install/PackageManager/PackageJSONEditor.rs
  • src/js_parser/function_identities.rs
  • src/js_parser/p.rs
  • src/js_parser/parse/mod.rs
  • src/js_parser/parse/parse_entry.rs
  • src/js_parser/parse/parse_stmt.rs
  • src/js_parser/parse/parse_suffix.rs
  • src/runtime/cli/mod.rs
  • src/runtime/cli/pm_diff_normalize.rs
💤 Files with no reviewable changes: 13
  • src/bundler/LinkerGraph.rs
  • src/js_parser/function_identities.rs
  • src/runtime/cli/pm_diff_normalize.rs
  • src/bundler/ParseTask.rs
  • src/js_parser/parse/parse_stmt.rs
  • src/js_parser/parse/mod.rs
  • src/bundler/error.rs
  • src/bundler/bundle_v2.rs
  • src/install/PackageManager/PackageJSONEditor.rs
  • src/js_parser/parse/parse_suffix.rs
  • src/js_parser/p.rs
  • src/bundler/transpiler.rs
  • src/js_parser/parse/parse_entry.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The changes remove bundler result and error variants, parser warning options and checks, and reason-based CLI help handling. They also remove a redundant package replacement check and the File default implementation.

Changes

Bundler Parse Results

Layer / File(s) Summary
Parse errors and result handling
src/bundler/ParseTask.rs, src/bundler/error.rs, src/bundler/bundle_v2.rs
ResultValue::Empty and the EmptyAST special case were removed. Parse failures now return error results with their error, step, log, source index, and target. ModuleNotFound and EmptyAST were removed from Error. BundleV2 no longer handles empty results.

Bundler File Default

Layer / File(s) Summary
File default implementation removal
src/bundler/LinkerGraph.rs
The File type’s Default implementation was removed.

JavaScript Parser Warning Changes

Layer / File(s) Summary
Parser options and warning checks
src/js_parser/parse/parse_entry.rs, src/js_parser/parse/mod.rs, src/js_parser/parse/parse_stmt.rs, src/js_parser/parse/parse_suffix.rs, src/js_parser/p.rs, src/js_parser/function_identities.rs, src/bundler/transpiler.rs, src/runtime/cli/pm_diff_normalize.rs
The parser option for suppressing unusual-code warnings was removed from Options and its call sites. The bare-return ASI warning and unfinished in and instanceof checks were removed. The parser no longer tracks whether the latest return had a semicolon.

CLI Help Output

Layer / File(s) Summary
Standard help printing and callers
src/runtime/cli/mod.rs
Help output no longer branches on explicit versus invalid-command reasons. HelpCommand::exec prints standard help and exits with status 0. Help tags use the shared print method.

Package JSON Replacement Check

Layer / File(s) Summary
Package replacement condition
src/install/PackageManager/PackageJSONEditor.rs
The keep_catalog_reference check was removed. The replacement condition still excludes the Update subcommand.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6ad24

No actionable behavior change is established; the PR is mergeable after normal checks.

Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: removing dead code across the bundler, JavaScript parser, help command, and PackageJSONEditor.
Description check Passed The description explains the problem, lists the removed code groups, documents the no-behavior-change intent, and provides detailed verification results. It does not use the template headings exactly,…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — every removed item is provably unreachable in the current tree, and the surviving paths are byte-for-byte the same.

What was reviewed:

  • Confirmed via repo-wide search that bun_bundler::Error::{EmptyAST, ModuleNotFound}, ResultValue::Empty, help_command::Reason, suppress_warnings_about_weird_code, and latest_return_had_semicolon have no remaining constructors/readers (remaining ModuleNotFound hits are the runtime/resolver/jsc crate enums, which are untouched; bun_transpiler re-exports the bundler enum and references neither variant).
  • Checked the Reason::InvalidCommand stderr/exit-1 path in the base commit: nothing constructed it, so no dispatch path loses the "Uh-oh" message; exec() still prints to stdout and exits 0.
  • Verified LinkerGraph::load zero-fills files via set_capacity/zero/set_len and no ..Default::default() or File::default() call exists, and that the dropped keep_catalog_reference term could only be true under subcommand == Update, which the preceding != Update operand already excludes.
  • The esbuild-ported ASI/!a in b warning stubs were gated off at every call site (option always true, or an empty TODO body), so no user-visible diagnostic changes.
Extended reasoning...

The change is a single commit of pure deletions (+23/-164) across the bundler error/parse-task plumbing, the JS parser options, the CLI help command, and PackageJSONEditor, with the only non-deletion being the mechanical collapse of print_with_reason into print. It touches no security-sensitive surface. Each removed item was independently re-verified as unconstructed or unread by searching the current tree, and the base-commit help code confirmed Reason::InvalidCommand was never produced. No CODEOWNERS entry covers the changed files, the bug hunt ran dry, and there are no outstanding third-party objections in the timeline.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants